Skip to content

Conversation

@holzman
Copy link

@holzman holzman commented Nov 4, 2025

@coderabbitai
Copy link

coderabbitai bot commented Nov 4, 2025

Walkthrough

The login options configuration for kubeconfig now enables insecure TLS when either the InsecureTLS flag is set or an existing insecure cluster is detected. Certificate error handling has been simplified to consistently prompt users for insecure TLS acceptance before retrying the connection.

Changes

Cohort / File(s) Change Summary
Login TLS and certificate handling
pkg/cli/login/loginoptions.go
Updated Insecure field initialization to check `o.InsecureTLS

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify hasExistingInsecureCluster() function exists and returns expected boolean values
  • Confirm the simplified certificate error handling path doesn't remove necessary security checks or bypass intended validations
  • Review the implications of always prompting for insecure TLS versus conditional prompting in the prior implementation
  • Validate test coverage for both the OR condition in Insecure initialization and the certificate error handling flow
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 1c5f490 and 19bbb26.

📒 Files selected for processing (1)
  • pkg/cli/login/loginoptions.go (2 hunks)
🔇 Additional comments (2)
pkg/cli/login/loginoptions.go (2)

186-202: LGTM! Simplified cert error handling is consistent with the initialization logic.

The simplified prompt logic is correct. Since line 163 already sets clientConfig.Insecure = true when an existing insecure cluster is detected, certificate errors should only occur when clientConfig.Insecure is false. In that case, prompting the user is the appropriate action.

This change eliminates redundant conditional checks and makes the error handling flow clearer.


163-163: Verification complete—hasExistingInsecureCluster is implemented correctly.

The function properly identifies clusters configured as insecure by checking InsecureSkipTLSVerify=true in the kubeconfig for matching hosts. The logic at line 163 correctly enables insecure TLS when either the user flag is set or an existing insecure cluster is detected, accomplishing the PR objective.


Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 4, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 4, 2025

Hi @holzman. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-ci openshift-ci bot requested review from ardaguclu and atiratree November 4, 2025 17:02
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 4, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: holzman
Once this PR has been reviewed and has the lgtm label, please assign atiratree for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ardaguclu
Copy link
Member

Thank you for your effort and time on this PR. This one #2134 contains unit test as well. So it is better to move forward with it.

In favor of #2134, closing...
/close

@openshift-ci openshift-ci bot closed this Nov 7, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 7, 2025

@ardaguclu: Closed this PR.

In response to this:

Thank you for your effort and time on this PR. This one #2134 contains unit test as well. So it is better to move forward with it.

In favor of #2134, closing...
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants